FINERACT-2432: LoanProductDataValidator.validateLoanScheduleType() fails silently when loanScheduleType is null or has an invalid value#5288
Conversation
wkigenyi
changed the title
| private void validateLoanScheduleType(final String transactionProcessingStrategyCode, final DataValidatorBuilder baseDataValidator, | ||
| final JsonElement element) { | ||
| final String loanScheduleType = this.fromApiJsonHelper.extractStringNamed(LoanProductConstants.LOAN_SCHEDULE_TYPE, element); | ||
| if (loanScheduleType == null) { |
There was a problem hiding this comment.
Having null value is not correct and it shall fail the validation and throw error!
There was a problem hiding this comment.
The sole purpose of the PR is to check if the value null was provided instead of leaving it out entirely or using undefined (remember it is not a required field), and if this is the case the validate function does not proceed (returns immediately). Is an integration test necessary?
There was a problem hiding this comment.
i think its required and mandatory field or it should be.
There was a problem hiding this comment.
According to the validation and the API docs loanScheduleType is not among the mandatory fields but it has a default value that is used in case it is not specified when creating a loan product ie "CUMULATIVE". I have added some tests to take care of this. I have also address a closely related issue where the loanScheduleType has an invalid value.
There was a problem hiding this comment.
if (this.fromApiJsonHelper.parameterExists(LoanProductConstants.LOAN_SCHEDULE_TYPE, element)) {
validateLoanScheduleType(transactionProcessingStrategyCode, baseDataValidator, element);
}
This is how it starts. If any value was provided for "loanScheduleType", it CANNOT be Null!
There was a problem hiding this comment.
All the rest of the changes can be removed.
There was a problem hiding this comment.
I agree. This is clean.
There was a problem hiding this comment.
It looks clean but it does not sort out the issue. If loanScheduleType is NULL and it is ignored then we shall have a NullPointer issue here if (! LoanScheduleType.PROGRESSIVE.equals(LoanScheduleType.valueOf(loanScheduleType))) because execution did not stop even after finding the the value is null.
There was a problem hiding this comment.
indeed, I guess we can keep this part
if (loanScheduleType == null || baseDataValidator.hasError()) {
return;
}
cfdf85a to
9b0a46f
Compare
wkigenyi
changed the title
wkigenyi
changed the title
| if (!LoanScheduleType.PROGRESSIVE.equals(LoanScheduleType.valueOf(loanScheduleType)) | ||
| if (loanScheduleType == null || baseDataValidator.hasError()) { | ||
| return; | ||
| } | ||
|
|
||
| LoanScheduleType scheduleType; | ||
| try { | ||
| scheduleType = LoanScheduleType.valueOf(loanScheduleType); | ||
| } catch (IllegalArgumentException e) { | ||
| throw new GeneralPlatformDomainRuleException("error.msg.invalid.loan.schedule.type", "Invalid loan schedule type"); | ||
| } | ||
|
|
||
| if (!LoanScheduleType.PROGRESSIVE.equals(scheduleType) |
There was a problem hiding this comment.
Not needed.
If the value is NULL just throw new PlatformApiDataValidationException...
Something like this:
Index: fineract-provider/src/main/java/org/apache/fineract/portfolio/loanproduct/serialization/LoanProductDataValidator.java
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/fineract-provider/src/main/java/org/apache/fineract/portfolio/loanproduct/serialization/LoanProductDataValidator.java b/fineract-provider/src/main/java/org/apache/fineract/portfolio/loanproduct/serialization/LoanProductDataValidator.java
--- a/fineract-provider/src/main/java/org/apache/fineract/portfolio/loanproduct/serialization/LoanProductDataValidator.java (revision dff889be29032cd75f18d2d418f28989f53c3cf5)
+++ b/fineract-provider/src/main/java/org/apache/fineract/portfolio/loanproduct/serialization/LoanProductDataValidator.java (date 1767962780072)
@@ -2797,8 +2797,9 @@
private void validateLoanScheduleType(final String transactionProcessingStrategyCode, final DataValidatorBuilder baseDataValidator,
final JsonElement element) {
final String loanScheduleType = this.fromApiJsonHelper.extractStringNamed(LoanProductConstants.LOAN_SCHEDULE_TYPE, element);
- baseDataValidator.reset().parameter(LoanProductConstants.LOAN_SCHEDULE_TYPE).value(loanScheduleType)
- .isOneOfEnumValues(LoanScheduleType.class);
+ baseDataValidator.reset().parameter(LoanProductConstants.LOAN_SCHEDULE_TYPE).value(loanScheduleType).notNull();
+ baseDataValidator.throwValidationErrors();
+ baseDataValidator.reset().parameter(LoanProductConstants.LOAN_SCHEDULE_TYPE).value(loanScheduleType).isOneOfEnumValues(LoanScheduleType.class);
if (!LoanScheduleType.PROGRESSIVE.equals(LoanScheduleType.valueOf(loanScheduleType))
&& AdvancedPaymentScheduleTransactionProcessor.ADVANCED_PAYMENT_ALLOCATION_STRATEGY
wkigenyi
force-pushed
the
FINERACT-2432
branch
from
9b0a46f to
27758d6
Compare
2 participants
Description
Describe the changes made and why they were made. (Ignore if these details are present on the associated Apache Fineract JIRA ticket.)
Checklist
Please make sure these boxes are checked before submitting your pull request - thanks!
Your assigned reviewer(s) will follow our guidelines for code reviews.